Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create & List Commands #135

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Create & List Commands #135

wants to merge 19 commits into from

Conversation

dmccoystephenson
Copy link
Member

@dmccoystephenson dmccoystephenson commented Mar 2, 2023

The changes in this PR allow players to create fiefs & view a list of all fiefs on the server.

Additional Changes

  • Added unit tests.

closes #116
closes #117

@dmccoystephenson
Copy link
Member Author

I've marked this as a draft for now since there's currently no link between a fief & its faction.

src/main/kotlin/com/dansplugins/fiefs/Fiefs.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/dansplugins/fiefs/fief/Fief.kt Outdated Show resolved Hide resolved
src/main/resources/plugin.yml Outdated Show resolved Hide resolved
src/test/kotlin/com/dansplugins/fiefs/fief/TestFief.kt Outdated Show resolved Hide resolved
@dmccoystephenson dmccoystephenson marked this pull request as ready for review March 12, 2023 06:19

class FiefsListCommand(private val plugin: Fiefs) : CommandExecutor, TabCompleter {
override fun onCommand(sender: CommandSender, command: Command, label: String, args: Array<out String>): Boolean {
if (plugin.fiefRepository.getFiefs().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the fief repository will read these from the database, you may wish to be careful about threading - commands are run on the main thread, and database queries perform I/O which would cause lag spikes each time the command was run.
Additionally, I'd recommend adding another layer of abstraction in a fief service type thing (which contains any domain logic) so your command isn't interfacing directly with the database.


data class Fief(private val name: String, private val ownerMfPlayerId: MfPlayerId, private val mfFactionid: MfFactionId) {
private var id = MfFiefId.generate()
private var members: MutableList<MfPlayerId> = mutableListOf()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you're replacing the entire list, this can be a val.
Additionally, if you use a List instead of a MutableList, this class can be immutable.
id and members could probably be moved into the constructor and given a default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow players in factions to create fiefs. Allow players to list fiefs.
2 participants